fix(personal): run directory indexing off the event loop - #5634
fix(personal): run directory indexing off the event loop#5634StressTestor wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
ready for review
|
I checked the latest head against current Findings
Validation
|
|
pushed 13c7f6d. moved the job lock to an asyncio.Lock acquired in the handler before offloading, and routed add, remove, and reload through it. that closes both halves: remove/reload are now serialized against an in-flight add (the inconsistent-state path), and a queued request parks on the event loop instead of pinning a threadpool worker (the starvation). remove and reload also run their blocking work off the loop now. added add-vs-remove and add-vs-reload serialization regressions, and converted the existing add-vs-add test to the same driver. they run async via ASGITransport because an asyncio.Lock deadlocks the sync TestClient portal (same reason test_notes_fail_closed_auth.py uses it). left the upload/delete sibling handlers and the partial-index-on-bookkeeping-failure window as separate follow-ups to keep this scoped. |
6c3b6d3 to
d8a2059
Compare
POST /api/personal/add_directory called rag.index_personal_documents inline from an async handler, so the whole indexing job (os.walk, file reads, per-chunk embedding, Chroma inserts) ran on the event loop and every other request queued behind it. Indexing a real directory froze the UI and API for 25+ minutes with no sign of life. Move the blocking section into the threadpool via run_in_threadpool. personal_docs_manager.add_directory stays inside it because its refresh_index() re-extracts text across tracked directories, which is also blocking work. A module-level lock serializes index jobs so the threadpool move does not introduce parallel jobs racing PersonalDocsManager's unsynchronized list mutations and file writes; they previously serialized on the blocked loop, so one-at-a-time is behavior parity.
The #5558 fix took the job lock INSIDE the threadpool worker and only on the add path, so (1) remove_directory and /reload mutated PersonalDocsManager's unsynchronized list/index concurrently with an in-flight add — the inconsistent state the PR claimed to prevent — and (2) a queued add blocked on the lock while holding an AnyIO threadpool token, starving the shared pool. Move the lock to an asyncio.Lock acquired in the async handler BEFORE offloading, and route add, remove and reload through it. A waiting request now parks on the event loop instead of pinning a worker, and all three mutators are serialized so the 'add/remove are serialized and cannot leave inconsistent state' guarantee holds. remove and reload also run their blocking work off the event loop. The lock is per-router so each app binds it to its own loop; single-process scope. Tests: add-vs-remove and add-vs-reload serialization regressions (async via ASGITransport, since asyncio.Lock deadlocks starlette TestClient's portal); the existing add-vs-add test converted to the same driver.
13c7f6d to
e222e92
Compare
2 participants
Summary
POST /api/personal/add_directoryis anasync defhandler that called the fully synchronousrag.index_personal_documentsinline, so the entire indexing job (os.walk, file reads, per-chunk embedding, Chroma inserts) ran on the event loop. Every other request queued behind it: indexing a real directory froze the UI and API for 25+ minutes with no sign of life (the reporter's users concluded the app crashed).This PR moves the blocking section into the threadpool via
fastapi.concurrency.run_in_threadpool.personal_docs_manager.add_directorystays inside the moved section because it triggersrefresh_index(), which re-extracts text across tracked directories, also blocking work. A module-level lock serializes index jobs: without it the threadpool move would let concurrent requests run parallel jobs that racePersonalDocsManager's unsynchronized list mutations and plainopen('w')file writes. Jobs previously serialized on the blocked loop anyway, so one-at-a-time is behavior parity, minus the freeze.Target branch
dev, notmain.Linked Issue
Fixes #5558
Type of Change
Checklist
devuvicorn app:app+ a local ChromaDB atlocalhost:8100) and verified the change works end-to-end.How to Test
python -m pytest tests/test_add_directory_event_loop.py: 4 tests. The thread-identity test asserts indexing and bookkeeping run off the event loop thread (fails ondev, where both run on it); the serialization test asserts concurrent requests never run parallel index jobs; the other two pin response shape and the error path.data/personal_docs/bigvault/with 150 markdown files,POST /api/personal/add_directory, and while the job is running, time aGET /api/personalfrom a second terminal.dev: the GET takes 3.2s, because it queues behind the whole remaining index job (with the reporter's 25-minute job, that GET waits minutes).indexed_count: 150, failed_count: 0.Also ran: full
python -m pytest(4674 passed, 3 skipped; the 5 failures are the docker-socket tests, which fail identically on cleandevon this machine, macOS with no docker socket) andpython -m compileall app.py core routes src services scripts tests.Scope notes
POST /reloadorDELETE /remove_directorymid-job could race the indexing thread's bookkeeping, a window that could not occur before because the blocked loop prevented any dispatch. Admin-only and narrow; the clean fix is a lock insidePersonalDocsManageritself, which i kept out of this PR to keep the diff reviewable. Can follow up.